-
Notifications
You must be signed in to change notification settings - Fork 305
Support multiple workers for NODEFS /wordpress mounts #2231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Adjusts a slight mistake in the condition: ```ts // This returns true if activeSute is undefined since // undefined is different than "none" activeSite?.metadata.storage !== 'none' ``` The goal is to only return `true` from selectSitesLoaded() if we have an active, non-temporary site
…sions (#83) ## Motivation for the change, related issues Our changelog workflow is currently broken because it is missing extra GitHub token secrets. If we add these secrets, we will have to update them occasionally. I think we may be able to do without and would like to try that. In addition, we need to backfill changelog entries while omitting links to PRs (since we cannot link to our private PRs). ## Implementation details This PR: - Updates our changelog update workflow to persist credentials in the local git config and attempts to follow the example of pushing a commit after actions/checkout: https://github.com/actions/checkout?tab=readme-ov-file#push-a-commit-using-the-built-in-token - Omits PR links from new changelog entries unless they point to github.com/WordPress/wordpress-playground - Backfills changelog entries for v1.0.25 to v1.0.29. ## Testing Instructions (or ideally a Blueprint) - Temporarily disable protections on running the workflow - Make the workflow target the PR branch - Manually run the workflow and see if it commits a changelog update to the PR branch - If successful, remove the changelog commit from the PR branch - Re-enable protections - Re-enable checking out trunk only - Merge
## Motivation for the change, related issues Testing Playground CLI with bun (via `npx nx dev playground-cli`) is much faster than building and running via node (via `npx nx start playground-cli`). This PR adds the option to run `@php-wasm/cli` the same way. ## Implementation details This PR adds a `dev` target to the php-wasm-cli project. The `dev` target runs `@php-wasm/cli` using `bun --watch`. ## Testing Instructions (or ideally a Blueprint) - CI - Manually try the new target with `npx nx dev php-wasm-cli "-r 'echo \"huzzah\n\";'"` - Note: For some reason, extra args need to be quoted because they are not escaped when forwarded, at least with our current version of nx and nx:run-commands.
Also add support for Node.js workers
I need to step away for the day but plan to resume in the morning. Will see what kind of compromises we can make to keep the big merges moving. |
I'm making some progress with the test-built-npm-packages tests. I've been able to run the CommonJS tests with the CLI server cleaning up and not causing hanging, but the tests for ES modules have been apparently conflicting with vitest and tinypool. IIUC, when a Playground worker is terminated, tinypool detects this as an unexpected exit. To avoid this issue and complexity, I'm working on just using the Node.js test framework which should be simpler and contain fewer surprises. It is working for a single PHP version but crashes when trying multiple PHP versions. Maybe testing built packages with ES modules for a single PHP version is good enough to merge this PR, especially since the CommonJS tests are testing all supported PHP versions. We could continue debugging this afterward. Will push my changes after a bit more troubleshooting. |
The built npm package tests are passing because I switched the ES module tests to a manual test runner that runs one test per process. Without that, the second invocation of runCLI() crashes the test process in both Vitest (regardless of configured pool type) and the builtin Node test runner. Using a manual test runner with a one-test-per-process approach seems a bit silly to me. But it works around the issue of Workers conflicting with the test runners. What is left:
@adamziel, if you are still interested in helping with this PR, 2 and 3 are up for grabs at the moment. We could actually fix these things or punt for a short time to enable merging XDebug and Blueprints v2 for Playground. |
I also haven't started looking at review comments because I've been digging into test failures. IIRC @adamziel said they weren't blockers, but I still intend to address them, even if in a follow-up PR. |
Recent work:
@adamziel I think we are in a place where we could merge this and then create a follow-up PR to address review comments. I also have an idea about what might be leading to Asyncify fd_close() crashes, so we can see about that as well. What do you think? Should we merge this, merge XDebug and Blueprints v2 support, and refine after? |
Edited the previous comment to add this line:
|
Let's merge :) |
Also, if vitest is so problematic, we could move to another library entirely (in a follow-up pr) |
## Motivation for the change, related issues #2231 overrides `FS.hashAddNode` with `function hashAddNodeIfNotSharedFS(node)` where additional logic applies if `is_shared_fs_node(node)` is true. Only NODEFS nodes were supposed to be considered as coming from a shared fs. Unfortunately, the internal logic of `is_shared_fs_node()` also returned true for MEMFS nodes. This caused a FS error 44 for the following operation where `runtime2` attempts to create a directory in a `/wordpress` directory mounted from `runtime1`: ```ts import { loadNodeRuntime } from "@php-wasm/node"; import { getLoadedRuntime } from "@php-wasm/universal"; const opts = { emscriptenOptions: { ENV: { DOCROOT: '/wordpress' } } }; const runtime1 = getLoadedRuntime(await loadNodeRuntime('8.3', opts)); runtime1.FS.mkdir("/wordpress"); const runtime2 = getLoadedRuntime(await loadNodeRuntime('8.3', opts)); runtime2.FS.mkdir("/wordpress"); runtime2.FS.mount( runtime2.PROXYFS, { root: '/wordpress', fs: runtime1.FS }, '/wordpress' ); // This works: // runtime1.FS.mkdir("/wordpress/wp-content"); // This doesn't: runtime2.FS.mkdir("/wordpress/wp-content"); ``` Specifically, the FS error 44 was triggered inside `is_shared_fs_node()` when calling NODEFS operations on these non-NODEFS nodes. ## Implementation details Adds a check confirming the shared node comes from NODEFS. ## Testing Instructions (or ideally a Blueprint) * Confirm the reproduction above works without errors. * Once #2285 lands, we'll be able to add a unit test cc @brandonpayton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I grabbed the unresolved concerns from this PR and added them to the follow up issue here:
#2293
Planning to work on that issue next.
packages/php-wasm/compile/php/phpwasm-emscripten-library-file-locking-for-node.js
Outdated
Show resolved
Hide resolved
#2317) ## Overview Adds multi-worker support for Node.js Asyncify builds to complement [JSPI support](#2231) and enable usage in Node < v23. Note this doesn't work with Asyncify in web browsers. This PR also adds `fileLockManager` support for `@php-wasm/cli` ## Implementation This PR is different from other Asyncify PRs in that it doesn't actually make things work with Asyncify. Instead, it switches to synchronous message passing when JSPI is unavailable. This way, the Asyncify builds never have to engage in stack switching around `fd_close()` or `fcntl()`. This wasn't the first choice, but getting the Asyncify builds right was just too challenging, so we had to use another approach. ### Synchronous message passing via Comlink > [!IMPORTANT] > This PR forks the Comlink library to add `afterResponseSent?: (ev: MessageEvent) => void` argument to the `expose()` function. The rest is unchanged. Comlink isn't getting many new PRs so skipping updates (or backporting occasionally) seems fine. Playground already uses Comlink for RPC between workers. This PR adds synchronous bindings via `exposeSync` and `wrapSync`. The specific technique of exchanging messages is described in https://github.com/adamziel/js-synchronous-messaging: - Worker A calls `postMessage()` on Worker B's MessagePort to start the RPC exchange. - Worker A uses [Atomics.wait](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/wait) (with a `SharedArrayBuffer`) to synchronously wait for a notification from the Worker B. - Worker A uses [receiveMessageOnPort](https://nodejs.org/api/worker_threads.html#workerreceivemessageonportport) to synchronously read the data sent by Worker B. For usage example, see [comlink-sync.spec.ts](https://github.com/WordPress/wordpress-playground/blob/042387f57846a046b98e5837956111f53fa862fa/packages/php-wasm/universal/src/lib/comlink-sync.spec.ts#L87). The upsides of this approach: * Saves dozens-to-hundreds of hours on debugging Asyncify issues * Increased reliability * Provides useful stack traces when errors do happen. The downsides: * Fragmentation: Both synchronous and asynchronous handlers exist to get the best our of both Asyncify and JSPI. * Node.js-only: This extension does not implement a Safari-friendly transport. SharedArrayBuffer is an option, but it requires more restrictive CORP+COEP headers which breaks, e.g., YouTube embeds. Synchronous XHR might work if we really need Safari support for one of the new asynchronous features, but other than that let's just skip adding new asynchronous WASM features to Safari until WebKit supports stack switching. * Message passing between workers is slow. Avoid using synchronous messaging for syscalls that are invoked frequently and handled asynchronously in the same worker. ### Dual channel support The Emscripten-built php.js requires either: * A synchronous or asynchronous `fileLockManager` – when JSPI is available * A synchronous `fileLockManager` – when JSPI is not available This is implemented with preprocessor directives, e.g. ``` #if ASYNCIFY == 2 return Asyncify.handleAsync(async () => { #endif // ..code.. #if ASYNCIFY == 2 }); #endif ``` Why support both methods and not always use synchronous calls? Because web browsers have no `receiveMessageOnPort` and can only handle asynchronous message passing. Supporting both sync and async message channels provides maximum compatibility. The only environment where `fileLockManager` is not supported is Safari. ## Testing Instructions (or ideally a Blueprint) ### Playground CLI Run Playground CLI server with 5 workers using Asyncify: ```shell node --disable-warning=ExperimentalWarning --experimental-strip-types --experimental-transform-types --import ./packages/meta/src/node-es-module-loader/register.mts ./packages/playground/cli/src/cli.ts server --experimental-multi-worker=5 --mount-before-install ./tmp/new-site:/wordpress ``` Create some posts, install some plugins, confirm it does not crash. Then do the same with JSPI and confirm everything continues to work: ```shell node --experimental-wasm-jspi --disable-warning=ExperimentalWarning --experimental-strip-types --experimental-transform-types --import ./packages/meta/src/node-es-module-loader/register.mts ./packages/playground/cli/src/cli.ts server --experimental-multi-worker=5 --mount-before-install ./tmp/new-site:/wordpress ``` ### PHP.wasm CLI Run the test script below and confirm it does not crash or corrupt the database: ```shell node --loader=./packages/meta/src/node-es-module -loader/loader.mts ./packages/php-wasm/cli/src/main.ts test.php ``` test.php ```php <?php $db = new SQLite3('./db.sqlite'); $db->exec('CREATE TABLE IF NOT EXISTS products ( id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL, price REAL NOT NULL, category TEXT NOT NULL )'); // Insert some product data $db->exec("INSERT INTO products (name, price, category) VALUES ('Laptop', 999.99, 'Electronics')"); $db->exec("INSERT INTO products (name, price, category) VALUES ('Coffee Mug', 12.50, 'Kitchen')"); $db->exec("INSERT INTO products (name, price, category) VALUES ('Notebook', 5.99, 'Office')"); $db->exec("INSERT INTO products (name, price, category) VALUES ('Headphones', 79.99, 'Electronics')"); $result = $db->query('SELECT * FROM products ORDER BY category, name'); while ($row = $result->fetchArray(SQLITE3_ASSOC)) { echo "- {$row['name']} ({$row['category']}): $" . number_format($row['price'], 2) . "\n"; } $db->close(); ``` --------- Co-authored-by: Adam Zieliński <[email protected]>
Motivation for the change, related issues
We want to support concurrent php-wasm workers, but concurrent workers can corrupt the SQLite DB without file locking.
Emscripten's libc provides dummy fcntl() and flock() lock implementations, so the API calls succeed without any locking taking place.
The PR implements:
/wordpress
dir.Implementation details
This is a big PR that:
--experimentalMultiWorker
arg/wordpress
directory.CPU_COUNT - 1
.--experimentalMultiWorker=7
.isSharedFS
flag to all NODEFS nodes. This way we can tell whether file-locking is needed and possible for an FS node, even if wrapped with PROXYFS.@php-wasm/node
loader. (Emscripten's getpid() always returns 42 😅)--experimentalTrace
arg that enables detail tracing messages. Currently, these messages are just for logging, but as long as there are no issues with the trace facility, we could add others.js_wasm_trace(format, ...args)
. The purpose of using the printf style is that no formatting has to take place whenjs_wasm_trace()
is called unless tracing is really enabled.wasm_trace()
. It relays its messages tojs_wasm_trace()
.Testing Instructions (or ideally a Blueprint)